Implement spectrum copy(), , = methods, `--scale` command-line option #285

ajjackson · 2023-09-01T11:52:48Z

Closes #219
In order to make the --scale option beautiful, tweak the Python API so that any Spectrum can by multiplied by a scalar Real using * or *=. (These are implemented with "dunder" methods __mul__ and __imul__ respectively.)

In practice we will usually use __imul__, so makes sense to define __mul__ using it, creating a fresh Spectrum to write into. In principle it would be more efficient to create one with an "empty" data array, but as there is lots of code making modified array copies, life is a lot simpler with a consistent copy() method used as a base for this stuff. If copying the data becomes a noticeable bottleneck we could add an empty=False argument that allows us to construct with e.g. y_data=np.empty_like(self.y_data)*ureg(self.y_data_unit), but for now assume YAGNI!

With the new Python/numpy/Pint versions we can use np.copy() directly on an array Quantity, which reduces unit-shuffling boilerplate. 😄

To-do

github-actions · 2023-09-01T12:28:17Z

Test Results

      22 files ±  0       22 suites ±0 55m 23s ⏱️ + 1m 56s
  1 049 tests +  9   1 043 ✔️ +  9   6 💤 ±0 0 ❌ ±0
10 430 runs +90 10 367 ✔️ +90 63 💤 ±0 0 ❌ ±0

Results for commit 23c0ae3. ± Comparison against base commit cfb9611.

♻️ This comment has been updated with latest results.

codecov-commenter · 2023-09-01T13:30:21Z

Codecov Report

Merging #285 (23c0ae3) into master (cfb9611) will increase coverage by 0.01%.
The diff coverage is 97.72%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #285      +/-   ##
==========================================
+ Coverage   95.78%   95.79%   +0.01%     
==========================================
  Files          28       28              
  Lines        3958     3992      +34     
  Branches      798      803       +5     
==========================================
+ Hits         3791     3824      +33     
- Misses         98       99       +1     
  Partials       69       69

Files Changed	Coverage Δ
euphonic/spectra.py	`95.16% <97.22%> (+0.04%)`	⬆️
euphonic/cli/dos.py	`89.33% <100.00%> (+0.29%)`	⬆️
euphonic/cli/intensity_map.py	`96.72% <100.00%> (+0.11%)`	⬆️
euphonic/cli/powder_map.py	`92.05% <100.00%> (+0.10%)`	⬆️
euphonic/cli/utils.py	`91.15% <100.00%> (+0.05%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ajjackson · 2023-09-01T15:24:51Z

API wrangling paid off, 11ab6cc was a very satisfying little commit to write 😅

oerc0122 · 2023-09-01T15:35:03Z

euphonic/spectra.py

-        return Spectrum1D(self.x_data, summed_y_data,
-                          x_tick_labels=self.x_tick_labels,
-                          metadata=metadata)
+        return Spectrum1D(np.copy(self.x_data),


Does this also want to be copyed?

A copy of Spectrum1DCollection would be a Spectrum1DCollection, but this method returns a Spectrum1D.

In principle we could make a copy from e.g. the first element of the collection, but then the constructor is getting called once to make the element from the y_data and again to copy it.

It feels "different enough" to justify breaking the pattern, but I don't have a strong opinion on whether it is better to do it one way or the other.

oerc0122 · 2023-09-01T15:36:03Z

tests_and_analysis/test/euphonic_test/test_spectrum1d.py

+        with pytest.raises(AssertionError):
+            check_spectrum1d(spec, spec * 2.)
+
+    def test_copy(self):


Is this something that wants to be fixtured?

Did you have a particular kind of fixture in mind?

I don't see an elegant way to parametrize all of these different operations, but pytest has a lot of features...

True, the awkward, ugly way to do it would be a list of tuples of ("attr", operation) and a loop through them, but that gets pretty ugly pretty quickly and cleanliness here is key.

Could maybe define the check as a kind of teardown operation and make lots of little tests, but that's pesky in its own way.

I could tidy this a bit by defining an assert_different function that hides the pytest.raises?

e.g.

def test_copy(self): def assert_different(spectrum, other) -> None: with pytest.raises(AssertionError): check_spectrum1dcollection(spectrum, other) spec = get_spectrum1dcollection('gan_bands.json') spec.metadata = {'Test': 'item', 'int': 1} spec_copy = spec.copy() # Copy should be same check_spectrum1dcollection(spec, spec_copy) # Until data is edited spec_copy._y_data *= 2 assert_different(spec, spec_copy) spec_copy = spec.copy() spec_copy._x_data *= 2 assert_different(spec, spec_copy) spec_copy = spec.copy() spec_copy.x_tick_labels = [(1, 'different')] assert_different(spec, spec_copy) spec_copy = spec.copy() spec_copy.metadata['Test'] = spec_copy.metadata['Test'].upper() assert_different(spec, spec_copy)

In practice we will usually want __imul__, so makes sense to define __mul__ using it. This requires a fresh Spectrum to write into. In principle it would be more efficient to create one with an "empty" data array... ... but as there is lots of code making modified array copies, life is a lot simpler with a consistent copy() method used as a base for this stuff. If copying the data becomes a noticeable bottleneck we could add an `empty=False` argument that allows us to construct with e.g. `y_data=np.empty_like(self.y_data)*ureg(self.y_data_unit)`, but for now assume YAGNI! With the new Python/numpy/Pint versions we can use np.copy() directly on an array Quantity, which reduces unit-shuffling boilerplate.

Shallow copies may lead to surprises when elements are mutated; _usually_ it will contain literals (and the type-hinting says it is _supposed_ to only take literals) but a user might put something mutable in there.

ajjackson · 2023-09-01T17:19:02Z

Rebasing onto the updated tests, not expecting drama but would be better to find out here.

oerc0122 approved these changes Sep 1, 2023

View reviewed changes

ajjackson marked this pull request as ready for review September 1, 2023 16:26

ajjackson requested a review from oerc0122 September 1, 2023 17:14

ajjackson added 11 commits September 1, 2023 18:16

Always deepcopy metadata dict

2e0c320

Shallow copies may lead to surprises when elements are mutated; _usually_ it will contain literals (and the type-hinting says it is _supposed_ to only take literals) but a user might put something mutable in there.

Spectrum refactoring

b063218

Unit test Spectrum.copy()

ac132fc

__imul__ needs return values; spectrum2D mul tests

f76b0da

Test spectrum multiplication

999b1af

Implement --scale option for dos, intensity-map and powder-map

41b81be

Add dos script test for --scale

3ae621e

Add intensity-map scipt test for --scale

fff7854

Add script test for powder-map --scale

57660c1

Update CHANGELOG.rst

23c0ae3

ajjackson force-pushed the scale branch from b3c9f24 to 23c0ae3 Compare September 1, 2023 17:18

oerc0122 approved these changes Sep 1, 2023

View reviewed changes

ajjackson merged commit 7042114 into master Sep 1, 2023
6 of 7 checks passed

ajjackson deleted the scale branch December 12, 2024 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement spectrum copy(), , = methods, `--scale` command-line option #285

Implement spectrum copy(), , = methods, `--scale` command-line option #285

ajjackson commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading

codecov-commenter commented Sep 1, 2023 •

edited

Loading

ajjackson commented Sep 1, 2023

oerc0122 Sep 1, 2023

ajjackson Sep 1, 2023

oerc0122 Sep 1, 2023

ajjackson Sep 1, 2023

oerc0122 Sep 1, 2023

ajjackson Sep 1, 2023

ajjackson Sep 1, 2023 •

edited

Loading

ajjackson commented Sep 1, 2023

Implement spectrum copy(), *, *= methods, --scale command-line option #285

Implement spectrum copy(), *, *= methods, --scale command-line option #285

Conversation

ajjackson commented Sep 1, 2023 • edited Loading

github-actions bot commented Sep 1, 2023 • edited Loading

Test Results

codecov-commenter commented Sep 1, 2023 • edited Loading

Codecov Report

ajjackson commented Sep 1, 2023

oerc0122 Sep 1, 2023

Choose a reason for hiding this comment

ajjackson Sep 1, 2023

Choose a reason for hiding this comment

oerc0122 Sep 1, 2023

Choose a reason for hiding this comment

ajjackson Sep 1, 2023

Choose a reason for hiding this comment

oerc0122 Sep 1, 2023

Choose a reason for hiding this comment

ajjackson Sep 1, 2023

Choose a reason for hiding this comment

ajjackson Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

ajjackson commented Sep 1, 2023

Implement spectrum copy(), , = methods, `--scale` command-line option #285

Implement spectrum copy(), , = methods, `--scale` command-line option #285

ajjackson commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading

codecov-commenter commented Sep 1, 2023 •

edited

Loading

ajjackson Sep 1, 2023 •

edited

Loading